Implement the basic Promise #1695

jiangzidong · 2017-03-29T02:55:34Z

Implement the Promise Constructor and routine: 'then'
add some promise test cases
add queue_init and queue_run in main-unix for both file mode and repl mode

In the spec of Promise, there are PromiseCapability Record and PromiseReaction Record. I use ecma_object_t to represent them cause it can reuse the current GC system

In the record object, I use UINT32 type string to represent each property names. I choose uint32 string for saving space. Or should I use LIT_INTERNAL_MAGIC_STRING_XXX instead?

Data Structure for Promise

Promise

In Spec:

Internal Slot	Description
[[PromiseState]]	A String value that governs how a promise will react to incoming calls to its then method. The possible values are: "pending", "fulfilled", and "rejected".
[[PromiseResult]]	The value with which the promise has been fulfilled or rejected, if any. Only meaningful if [[PromiseState]] is not "pending".
[[PromiseFulfillReactions]]	A List of PromiseReaction records to be processed when/if the promise transitions from the "pending" state to the "fulfilled" state.
[[PromiseRejectReactions]]	A List of PromiseReaction records to be processed when/if the promise transitions from the "pending" state to the "rejected" state.

In Jerry

typedef struct
{
  ecma_extended_object_t ecma_extended_object_t; /**< extended object part */
  uint8_t state; /**< promise state, see ecma_promise_state_t */
  ecma_collection_header_t *fulfill_reactions; /**< list of PromiseFullfillReactions */
  ecma_collection_header_t *reject_reactions; /**< list of PromiseRejectReactions */
} ecma_promise_object_t;

And it is a CLASS type object, and its ext_object_p->u.class_prop.u.value stores the promiseResult
Each elements in fulfill_reactions and reject_reactions are also ecma_object.
Those internal property are not accessible by js code. so js users will not read/write those values via getter/setter of promise object

PromiseReaction Record

It is the elements of promise.[[PromiseFulfillReactions]] and [[PromiseRejectReactions]]

In Spec:

Field Name	Value	Meaning
[[Capabilities]]	A PromiseCapability record	The capabilities of the promise for which this record provides a reaction handler.
[[Handler]]	A function object or a String	The function that should be applied to the incoming value, and whose return value will govern what happens to the derived promise. If [[Handler]] is "Identity" it is equivalent to a function that simply returns its first argument. If [[Handler]] is "Thrower" it is equivalent to a function that throws its first argument as an exception.

In Jerry:

an GENERAL_TYPE object with following properties:
"0": the PromiseCapability object
"1": handler, could be function or boolean value. If it is boolean, true means "identity" and false means "thrower"

PromiseCapability Records

In spec:

Field Name	Value	Meaning
[[Promise]]	An object	An object that is usable as a promise.
[[Resolve]]	A function object	The function that is used to resolve the given promise object.
[[Reject]]	A function object	The function that is used to reject the given promise object.

In Jerry:

Similar with PromiseReaction , a general object to represent, with uint32_string properties

"0": the promise object
"1": resolve function
"2": reject function

zherczeg

First round of review. Pretty good patch. But please be more careful about syntax rules. Just self-review your code.

zherczeg · 2017-03-29T09:25:58Z

jerry-core/ecma/builtin-objects/ecma-builtin-promise-prototype.inc.h

+ */
+#ifndef OBJECT_VALUE
+# define OBJECT_VALUE(name, obj_builtin_id, prop_attributes)
+#endif /* !OBJECT_VALUE */


The new model is calling this include pair:

#include "ecma-builtin-helpers-macro-defines.inc.h" #include "ecma-builtin-helpers-macro-undefs.inc.h"

zherczeg · 2017-03-29T09:30:20Z

jerry-core/ecma/builtin-objects/ecma-builtin-promise-prototype.c

+#include "ecma-promise-object.h"
+#include "ecma-try-catch-macro.h"
+#include "jrt.h"
+#include "jrt-libc-includes.h"


Do we need all of these includes?

zherczeg · 2017-03-29T09:32:06Z

jerry-core/ecma/builtin-objects/ecma-builtin-promise.c

+#include "ecma-helpers.h"
+#include "ecma-promise-object.h"
+#include "ecma-try-catch-macro.h"
+#include "jrt.h"


Please drop unnecessary includes in all files.

zherczeg · 2017-03-29T09:34:15Z

jerry-core/ecma/operations/ecma-jobqueue.c

+ * @return pointer to the PromiseReactionJob
+ */
+static ecma_job_promise_reaction_t *
+create_promise_reaction_job (ecma_value_t reaction, /**< PromiseReaction */


These functions do not have ecma_ or ecma_job_ prefix. Even if they are static functions.

zherczeg · 2017-03-29T09:37:46Z

jerry-core/ecma/operations/ecma-jobqueue.c

+
+  ecma_string_t *str_0 = ecma_new_ecma_string_from_uint32 (0);
+  ecma_string_t *str_1 = ecma_new_ecma_string_from_uint32 (1);
+  ecma_string_t *str_2 = ecma_new_ecma_string_from_uint32 (2);


Why do you use indices instead of names? Do they come from the standard?

In spec, they are "record" instead of object. I use object to represent them so that I can use of the current gc system instead of creating my own.
The relationship between spec definition and my design are list in the first post #1695 (comment)

Only internal code will use those properties. I choose indices to save space and time (create string from c char is less effecient) But I realized maybe I can use LIT_INTERNAL_MAGIC_STRING_XXX to represent the name. What do you think about it?

Hm, numbers are ok for me. But you can use symbolic strings if you want.

zherczeg · 2017-03-29T09:45:22Z

jerry-core/ecma/operations/ecma-promise-object.c

+ * @return ecma value
+ */
+static ecma_value_t
+trigger_promise_reactions (ecma_collection_header_t *reactions, /**< lists of reactions */


ecma_ prefixes are missing

zherczeg · 2017-03-29T09:45:45Z

jerry-core/ecma/operations/ecma-promise-object.c

+  ecma_promise_object_t *promise_p = (ecma_promise_object_t *) obj_p;
+  ecma_value_t ret = trigger_promise_reactions (promise_p->reject_reactions, reason);
+  promise_p->reject_reactions = ecma_new_values_collection (NULL, 0, false);
+  /* Free all fullfill_reactions */


Dot after sentence.

zherczeg · 2017-03-29T09:46:03Z

jerry-core/ecma/operations/ecma-promise-object.c

+} /* fulfill_promise */
+
+
+/**


Two newlines.

zherczeg · 2017-03-29T09:48:04Z

jerry-core/ecma/operations/ecma-promise-object.c

+ */
+static ecma_value_t
+resolve_function_handler (const ecma_value_t function,
+                          const ecma_value_t this __attribute__((unused)),


We don't use GCC specific __attribute__ ((unused)). Please use JERRY_UNUSED

zherczeg · 2017-03-29T09:49:49Z

jerry-core/ecma/operations/ecma-promise-object.c

+ * See also: ES2015 25.4.3.1
+ *
+ * @return ecma value of the new promise object
+ *         Returned value must be freed with ecma_free_value


returned instead of Returned

My bad, this should not be changed.

jiangzidong · 2017-03-30T05:27:05Z

@akosthekiss Hi, the patch can pass all check in ubuntu, but failed in macOS.
Could you give my some clues that which behaviors may be different in Mac and Ubuntu? I don't have mac in my hands, and I can't debug on that, but the clues may reminds me of the potential issue. Thanks

jiangzidong · 2017-03-30T06:14:11Z

@akosthekiss I found the reason. I set no_pure functions as attr__pure by mistake. e.g.

/**
 * Set the PromiseState of promise.
 */
inline void __attr_pure___ __attr_always_inline___
ecma_promise_set_state (ecma_object_t *obj_p, /**< points to promise object */
                        uint8_t state) /**< the state */
{
  JERRY_ASSERT (ecma_is_promise (obj_p));

  ((ecma_promise_object_t *) obj_p)->state = state;
} /* ecma_promise_set_state */

it actually changes some values in the heap.

In Ubuntu, the issue is not revealed. The GCC might do different optimizations for those 2 OS.

akosthekiss · 2017-03-30T10:21:11Z

@jiangzidong It's great that you've found the cause. Although I'm a bit confused as that function should count as pure (at least in release builds, when asserts are disabled), or do I get something wrong?

BTW, on macOS, gcc is not gcc but a wrapper around clang/llvm. So they may easily do things differently.

LaszloLango · 2017-03-30T10:35:37Z

jerry-core/ecma/operations/ecma-jobqueue.h

+{
+  ecma_value_t reaction;
+  ecma_value_t argument;
+} ecma_job_promise_reaction_t;


missing comments

LaszloLango · 2017-03-30T10:35:42Z

jerry-core/ecma/operations/ecma-jobqueue.h

+  ecma_value_t promise;
+  ecma_value_t thenable;
+  ecma_value_t then;
+} ecma_job_promise_resolve_thenable_t;


LaszloLango · 2017-03-30T10:37:49Z

jerry-core/ecma/operations/ecma-promise-object.c

+/**
+ * Check if alreadyResolved is true.
+ *
+ * @return bool


Please add a better description to the return value.

LaszloLango · 2017-03-30T10:38:16Z

jerry-core/ecma/operations/ecma-promise-object.c

+ *
+ * See also: ES2015 25.4.1.8
+ *
+ * @return ecma value


LaszloLango · 2017-03-30T10:38:40Z

jerry-core/ecma/operations/ecma-promise-object.c

+ *
+ * See also: ES2015 25.4.1.7
+ *
+ * @return ecma value


LaszloLango · 2017-03-30T10:40:49Z

jerry-core/ecma/operations/ecma-promise-object.c

+ecma_promise_reject_handler (const ecma_value_t function,
+                             const ecma_value_t this,
+                             const ecma_value_t argv[],
+                             const ecma_length_t argc)


missing comments

LaszloLango · 2017-03-30T10:42:12Z

jerry-core/ecma/operations/ecma-promise-object.c

+ecma_promise_resolve_handler (const ecma_value_t function,
+                              const ecma_value_t this,
+                              const ecma_value_t argv[],
+                              const ecma_length_t argc)


LaszloLango · 2017-03-30T10:42:25Z

jerry-core/ecma/operations/ecma-promise-object.c

+ecma_promise_executor_handler (const ecma_value_t function,
+                               const ecma_value_t this,
+                               const ecma_value_t argv[],
+                               const ecma_length_t argc)


LaszloLango · 2017-03-30T10:46:10Z

jerry-core/ecma/operations/ecma-promise-object.c

+  if (resolve != ecma_make_simple_value (ECMA_SIMPLE_VALUE_UNDEFINED))
+  {
+    ret = ecma_raise_type_error (ECMA_ERR_MSG ("'resolve' function should be undefined."));
+    goto end_of_executor_function;


I'm not completely against goto statements, but I think it can be avoided here.

yes, I can remove this goto.
What about the goto in ecma_promise_resolve_handler ? I think that one is needed, otherwise the code will be quite 'dirty'

I saw that too, but I agree. The code would be much less readable if you eliminate that one.

LaszloLango · 2017-03-30T10:48:58Z

jerry-core/ecma/operations/ecma-promise-object.h

+} ecma_promise_object_t;
+
+
+


Too many empty lines. One is enough :)

jiangzidong · 2017-04-01T06:51:00Z

@akosthekiss Sorry that I deleted my last comment by accident.

inline void __attr_pure___ __attr_always_inline___
ecma_promise_set_state (ecma_object_t *obj_p, 
                        uint8_t state) 
{
  JERRY_ASSERT (ecma_is_promise (obj_p));

  ((ecma_promise_object_t *) obj_p)->state = state; //!!!HERE, change the value of memory heap
}

I don't think the above function is a pure function. It has side affect, ((ecma_promise_object_t *) obj_p)->state = state this statement will change the outer context's value (it actually changes the global variable's value)

zherczeg

Next round of review, questions.

zherczeg · 2017-04-03T08:38:17Z

jerry-core/ecma/base/ecma-gc.c

+        if (ext_object_p->u.class_prop.class_id == LIT_MAGIC_STRING_PROMISE_UL)
+        {
+          /* mark promise result */
+          ecma_value_t result = ext_object_p->u.class_prop.u.value;


Mark promise result.

zherczeg · 2017-04-03T08:38:56Z

jerry-core/ecma/base/ecma-gc.c

+          {
+            ecma_gc_set_object_visited (ecma_get_object_from_value (result), true);
+          }
+          /* mark all reactions */


Mark all reactions.
And add a newline before it.

zherczeg · 2017-04-03T08:44:19Z

jerry-core/ecma/operations/ecma-promise-object.c

+
+  /* 4. */
+  ecma_object_t *executor_p;
+  executor_p = ecma_op_create_external_function_object ((ecma_external_pointer_t) ecma_promise_executor_handler);


This seems to be a static function which is always the same. If this is a private opration, why do we need a JS representation for it?

Yes, I should directly call the executor instead of creating js function.

zherczeg · 2017-04-03T08:52:55Z

jerry-core/ecma/operations/ecma-jobqueue.c

+  ecma_job_promise_reaction_t *job_p;
+  job_p = (ecma_job_promise_reaction_t *) jmem_heap_alloc_block (sizeof (ecma_job_promise_reaction_t));
+
+  job_p->reaction = ecma_copy_value (reaction);


In the code below it seems reaction is always an object. We should add an assert here.

zherczeg · 2017-04-03T08:56:28Z

jerry-core/ecma/operations/ecma-jobqueue.c

+  if (ecma_is_value_false (handler) || ECMA_IS_VALUE_ERROR (handler_result))
+  {
+    /* str '2' indicates [[Reject]] of Capability */
+    handler_result = handler_result & ~ECMA_VALUE_ERROR_FLAG;


ecma_get_value_from_error_value should be called here.

Thanks! I didn't noticed this api

zherczeg · 2017-04-03T09:16:52Z

jerry-core/ecma/operations/ecma-promise-object.c

+
+  ecma_promise_object_t *promise_p = (ecma_promise_object_t *) obj_p;
+  ecma_promise_trigger_reactions (promise_p->reject_reactions, reason);
+  promise_p->reject_reactions = ecma_new_values_collection (NULL, 0, false);


Can't this be a NULL value?

Yes, it can. But if so, when we add reactions to the list, we have to create a collection when the value is NULL

zherczeg · 2017-04-03T09:17:56Z

jerry-core/ecma/operations/ecma-promise-object.c

+  ecma_free_values_collection (promise_p->fulfill_reactions, false);
+  promise_p->fulfill_reactions = ecma_new_values_collection (NULL, 0, false);
+
+  ecma_promise_set_state (obj_p, ECMA_PROMISE_STATE_REJECTED);


Qestion: is it possible that a callback triggers the same promise again? If so, this state set must occur after the assertion.

No. But I will put the set after the assertion to reduce misunderstanding.

zherczeg · 2017-04-03T09:18:17Z

jerry-core/ecma/operations/ecma-promise-object.c

+  ecma_free_values_collection (promise_p->reject_reactions, false);
+  promise_p->reject_reactions = ecma_new_values_collection (NULL, 0, false);
+
+  ecma_promise_set_state (obj_p, ECMA_PROMISE_STATE_FULFILLED);


Same comments as above.

zherczeg · 2017-04-03T09:24:19Z

jerry-core/ecma/operations/ecma-promise-object.c

+  /* 1. */
+  JERRY_ASSERT (ecma_is_promise (ecma_get_object_from_value (promise)));
+  /* 3. */
+  ecma_value_t already_resolved = ecma_op_object_get (function_p, str_1);


I don't really understand this part. The function_p is a public function where we define a "1" property? What ensures that we can define this property?

I should use LIT_INTERNAL_MAGIC_STRING_XXX here to prevent outer user from getting access to the internal property.

zherczeg · 2017-04-03T09:26:17Z

jerry-core/ecma/operations/ecma-promise-object.c

+  ecma_string_t *str_1 = ecma_new_ecma_string_from_uint32 (1);
+  /* 2. */
+  ecma_object_t *resolve_p;
+  resolve_p = ecma_op_create_external_function_object ((ecma_external_pointer_t) ecma_promise_resolve_handler);


This is again seems to be a case where we have fixed internal handlers, there is no point creating JS functions for them, just call them directly whenever they needed. Creating JS functions is costly, and calling them is slower than natively calling them.

1). resolve_handler and reject_handler are not used internally, it will be used as args of executor, which is passed by user to create a Promise. So I think it is needed to create JS function for ecma_promise_resolve_handler and ecma_promise_reject_handler.
e.g.

var a = new Promise (function(f, r) // here f is the resolve function and r is the reject function { xxxx, f(); // user control the logic of calling f or r });

2). you're right in your last comment, because reject and resolve will be passed to the user's function, we cannot define a normal string property for function_p. I will use LIT_INTERNAL_MAGIC_STRING_XXX as the property name, so that outer user can not get access to the property.

3).While the creation of ecma_promise_executor_handler is not necessary, and I will change it.

zherczeg · 2017-04-06T06:48:44Z

jerry-core/ecma/base/ecma-gc.c

@@ -179,7 +182,15 @@ ecma_gc_mark_property (ecma_property_pair_t *property_pair_p, /**< property pair
      if (ECMA_PROPERTY_GET_NAME_TYPE (property) == ECMA_STRING_CONTAINER_MAGIC_STRING
          && property_pair_p->names_cp[index] >= LIT_NON_INTERNAL_MAGIC_STRING__COUNT)
      {
+#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN
+        if (property_pair_p->names_cp[index] < LIT_INTERNAL_MAGIC_STRING_PROMISE)


We need to do this better, since this case would be too specialized for promises. We need to introduce a constant for those internal strings, which property value must not be marked. Promise magic string ID should be lower than this constant.

zherczeg · 2017-04-06T06:49:20Z

jerry-core/ecma/base/ecma-gc.c

+
+        if (ext_object_p->u.class_prop.class_id == LIT_MAGIC_STRING_PROMISE_UL)
+        {
+          /* Mark promise result */


The dot is still missing after the sentences.

zherczeg · 2017-04-06T06:52:56Z

jerry-core/ecma/builtin-objects/ecma-builtin-promise.c

+
+  if (arguments_list_len == 0 || !ecma_op_is_callable (arguments_list_p[0]))
+  {
+    return ecma_raise_type_error (ECMA_ERR_MSG ("first parameter must be callable."));


In the ECMA_ERR_MSG above you started the sentence with a capital letter, here with a small letter. Please use only one style, which is used in the other part of the project.

zherczeg · 2017-04-06T06:54:50Z

jerry-core/ecma/operations/ecma-jobqueue.c

+    ecma_free_value (reject);
+  }
+  /* 8 */
+  else


Don't use comment before else, put it inside the {} block.

zherczeg · 2017-04-06T06:56:35Z

jerry-core/ecma/operations/ecma-promise-object.c

+
+  JERRY_ASSERT (ext_object_p->u.class_prop.class_id == LIT_MAGIC_STRING_BOOLEAN_UL);
+
+  return ext_object_p->u.class_prop.u.value == ecma_make_boolean_value (true);


Are these functions user supplied?

zherczeg · 2017-04-06T06:56:59Z

jerry-core/ecma/operations/ecma-promise-object.c

+ */
+static void
+ecma_reject_promise (ecma_value_t promise, /**< promise */
+                ecma_value_t reason) /**< reason for reject */


Indentation.

zherczeg · 2017-04-06T06:59:00Z

jerry-core/ecma/operations/ecma-promise-object.c

+ */
+static void
+ecma_fulfill_promise (ecma_value_t promise, /**< promise */
+                 ecma_value_t value) /**< fulfilled value */


Indentation.

zherczeg · 2017-04-06T07:01:43Z

jerry-core/ecma/operations/ecma-promise-object.c

+  ecma_init_ecma_magic_string (&str_already_resolved, LIT_INTERNAL_MAGIC_STRING_ALREADY_RESOLVED);
+  /* 2. */
+  ecma_object_t *resolve_p;
+  resolve_p = ecma_op_create_external_function_object ((ecma_external_pointer_t) ecma_promise_resolve_handler);


Are these functions can be called by users? They seem internal based on their name. I would prefer just directly call them.

Please see my comments below

jiangzidong · 2017-04-06T08:27:58Z

Are these functions user supplied?
Are these functions can be called by users? They seem internal based on their name. I would prefer just directly call them.

This pair of functions (resolve function and reject function) are not supplied by user, but will pass to user as arguments. JS engine prepared/implemented these 2 functions, and let users to use them. For JS users, they are standard js functions. for example

var a = new Promise(function(f, r)  //here f is the resolve function, and r is the reject funtion. They are prepared by engine
{
  // user totally control how to use these 2 functions
  ......
  if (...)
  {
    f (x); // if some condition meets, resolve the promise
  } else {
    r (y); // otherwise, reject it
  }
})

jiangzidong · 2017-04-07T06:27:59Z

Updated for the 2nd review

LaszloLango

LGTM

LaszloLango · 2017-04-10T11:13:15Z

jerry-core/ecma/operations/ecma-jobqueue.c

+ */
+static void
+ecma_free_promise_resolve_thenable_job (ecma_job_promise_resolve_thenable_t *job_p) /**< points to the
+                                                                                 *  PromiseResolveThenableJob */


wrong indentation

zherczeg

LGTM

zherczeg · 2017-04-11T12:40:21Z

jerry-core/jerry-port.h

@@ -142,6 +142,8 @@ double jerry_port_get_current_time (void);

 #ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN

+#define JERRY_PORT_ENABLE_JOBQUEUE


Do we still need this define?

The marco is for main-unix.c, since I added the jobqueue related code in it. Also the jerry-port-default-jobqueue.c used the macro.

Those two files are not in the jerry-core, so the profile config doesn't work on them

Does it make sense, can I merge the patch? @zherczeg

Ok, lets merge it, and we can address such things later.

Implement the Promise Constructor and routine: 'then' JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

jiangzidong force-pushed the promise branch 2 times, most recently from 36d47bb to ae5ed3f Compare March 29, 2017 04:59

jiangzidong mentioned this pull request Mar 29, 2017

ES6 Promise #1675

Closed

jiangzidong added the ES2015 Related to ES2015 features label Mar 29, 2017

zherczeg requested changes Mar 29, 2017

View reviewed changes

jiangzidong force-pushed the promise branch 4 times, most recently from 6b222bb to 331716c Compare March 30, 2017 04:54

jiangzidong force-pushed the promise branch from 331716c to 2f15697 Compare March 30, 2017 05:44

LaszloLango requested changes Mar 30, 2017

View reviewed changes

jiangzidong force-pushed the promise branch 2 times, most recently from 5b6e6e1 to 7466381 Compare March 30, 2017 13:29

zherczeg requested changes Apr 3, 2017

View reviewed changes

zherczeg requested changes Apr 6, 2017

View reviewed changes

jiangzidong force-pushed the promise branch from 5f3f85e to 585464a Compare April 6, 2017 10:47

jiangzidong force-pushed the promise branch from 585464a to d525650 Compare April 10, 2017 10:41

LaszloLango approved these changes Apr 10, 2017

View reviewed changes

jiangzidong force-pushed the promise branch from d525650 to b19ac35 Compare April 10, 2017 11:42

zherczeg approved these changes Apr 11, 2017

View reviewed changes

jiangzidong force-pushed the promise branch from b19ac35 to 9388109 Compare April 12, 2017 12:58

Implement the basic Promise

9388109

Implement the Promise Constructor and routine: 'then' JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

jiangzidong merged commit eb8dd46 into jerryscript-project:master Apr 12, 2017

jiangzidong deleted the promise branch April 12, 2017 14:12

akosthekiss mentioned this pull request Apr 15, 2017

Ensure that const and pure function attributes are used properly #1739

Merged


		JERRY_ASSERT (ext_object_p->u.class_prop.class_id == LIT_MAGIC_STRING_BOOLEAN_UL);

		return ext_object_p->u.class_prop.u.value == ecma_make_boolean_value (true);

		@@ -142,6 +142,8 @@ double jerry_port_get_current_time (void);

		#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN

		#define JERRY_PORT_ENABLE_JOBQUEUE

Implement the basic Promise #1695

Implement the basic Promise #1695

Uh oh!

Conversation

jiangzidong commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Data Structure for Promise

Promise

PromiseReaction Record

PromiseCapability Records

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong commented Mar 30, 2017

Uh oh!

jiangzidong commented Mar 30, 2017

Uh oh!

akosthekiss commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong commented Apr 1, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jiangzidong commented Mar 29, 2017 •

edited

Loading